Skip to content

Conversation

@anakaren-rojas
Copy link
Contributor

@anakaren-rojas anakaren-rojas commented Jan 22, 2026

Summary:

JSON editor data wasn't being synced with Editor Page data

JSON Editor

  • Updated JSON editor to use modern lifecycle method
  • Added condition to check whether value prop changed before updating state
  • Added tests for JSON Editor

Editor Page

  • Added getSnapshotBeforeUpdate lifecycle method to capture editor changes before unmounting
  • Use snapshot to update json when json toggled

Issue: LEMS-3701

Test plan:

Screen recordings

Before

Screen.Recording.2026-01-26.at.16.28.27.mov

After

Screen.Recording.2026-01-26.at.16.29.14.mov

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

🗄️ Schema Change: No Changes ✅

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (ac23e42) and published it to npm. You
can install it using the tag PR3196.

Example:

pnpm add @khanacademy/perseus@PR3196

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR3196

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR3196

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Size Change: +186 B (+0.04%)

Total Size: 485 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 99 kB +186 B (+0.19%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 20.8 kB
packages/keypad-context/dist/es/index.js 1 kB
packages/kmath/dist/es/index.js 5.98 kB
packages/math-input/dist/es/index.js 98.5 kB
packages/math-input/dist/es/strings.js 1.61 kB
packages/perseus-core/dist/es/index.item-splitting.js 12.1 kB
packages/perseus-core/dist/es/index.js 25 kB
packages/perseus-linter/dist/es/index.js 8.84 kB
packages/perseus-score/dist/es/index.js 9.32 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/index.js 187 kB
packages/perseus/dist/es/strings.js 7.44 kB
packages/pure-markdown/dist/es/index.js 1.39 kB
packages/simple-markdown/dist/es/index.js 6.71 kB

compressed-size-action

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

🛠️ Item Splitting: No Changes ✅

@anakaren-rojas anakaren-rojas changed the title fix(LEMS-3701): update editor state fix(LEMS-3701): update editorPage to sync json state Jan 22, 2026
@anakaren-rojas anakaren-rojas marked this pull request as ready for review January 27, 2026 00:58
@anakaren-rojas anakaren-rojas requested a review from a team January 27, 2026 00:58
});
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads much better! Thank you!!

componentDidUpdate() {
getSnapshotBeforeUpdate(prevProps: Props, prevState: State) {
if (!prevProps.jsonMode && this.props.jsonMode) {
return _.extend(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding an underscore function (I think we are wanting to eventually get rid of underscore), could we write this as:

if (!prevProps.jsonMode && this.props.jsonMode) {
    const serializedItemEditor = this.itemEditor.current?.serialize({
        keepDeletedWidgets: true,
    }) || {};
    const serializedHintsEditor = hints: this.hintsEditor.current?.serialize({
        keepDeletedWidgets: true,
    });
    return {...serializedItemEditor, ...serializedHintsEditor};
}
return null;

Copy link
Contributor

@ivyolamit ivyolamit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anakaren-rojas here's my initial comment/question. Will do a second pass and smoke test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove code in line 2, if UNSAFE tag is removed

/* eslint-disable react/no-unsafe */

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish ESLint would tell us if there's "disable" statements that are unused. 🤷‍♂️

};
}

UNSAFE_componentWillReceiveProps(nextProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this was marked UNSAFE?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update, I asked Claude about this:

  • UNSAFE_componentWillReceiveProps = unsafe (has issues with concurrent rendering)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"componentWillReceiveProps was problematic because it was called before every render triggered by a prop change, making it difficult to distinguish between essential updates and potentially causing infinite loops or unpredictable behavior, especially with asynchronous operations. "

😓

syncJsonStateFromProps() {
this.setState({
// @ts-expect-error - TS2322 - Type 'Pick<Readonly<Props> & Readonly<{ children?: ReactNode; }>, "hints" | "question" | "answerArea">' is not assignable to type 'PerseusJson'.
json: _.pick(this.props, "question", "answerArea", "hints"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the following (to avoid adding underscore):

syncJsonStateFromProps() {
    this.setState({
        json: {
            question: this.props.question,
            answerArea: this.props.answerArea,
            hints: this.props.hints,
        },
    });
}

);

// Assert
expect(screen.getByDisplayValue(/Updated content/)).toBeInTheDocument();
Copy link
Contributor

@mark-fitzgerald mark-fitzgerald Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also assert that "Initial content" is NOT in the document?


await userEvent.clear(textarea);
textarea.focus();
await userEvent.paste('{"content": "User is typing');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing something - how is this invalid user input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's incomplete, its missing the " and the closing bracket
{"content": "User is typing
instead of:
{"content": "User is typing"}

expect(screen.getByRole("textbox")).toBeDisabled();
});

it("should replace invalid user input when parent updates value", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for valid user input when the parent updates the value?

Copy link
Contributor

@mark-fitzgerald mark-fitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!!

Copy link
Collaborator

@jeremywiebe jeremywiebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense. Thanks for fixing this! I think I've doubled-up on some of Mark's comments but I'd typed them before seeing his. :/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish ESLint would tell us if there's "disable" statements that are unused. 🤷‍♂️

this.state.currentValue ? this.state.currentValue : "",
),
);
componentDidUpdate(prevProps: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this function is used to update state, could we just use the static getDerivedStateFromProps instead? I'm not sure it's a huge difference, but it's a static method (which is nice as it is then a pure function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion!
I looked into it and it doesn't seem like it would work for our use case 😞
The problem we were having was that the JsonEditor wasn't syncing when the parent sent updated props (whether on load or later), so we need ongoing prop synchronization

With componentDidUpdate, we're checking if the value prop from the parent has changed, not just on initial load but throughout the component's lifetime

}

componentDidUpdate() {
getSnapshotBeforeUpdate(prevProps: Props, prevState: State) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: Cool, I hadn't seen this React function before.

Comment on lines 127 to 136
return _.extend(
this.itemEditor.current?.serialize({
keepDeletedWidgets: true,
}) || {},
{
hints: this.hintsEditor.current?.serialize({
keepDeletedWidgets: true,
}),
},
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a simple object spread here instead of introducing another use of _?

Suggested change
return _.extend(
this.itemEditor.current?.serialize({
keepDeletedWidgets: true,
}) || {},
{
hints: this.hintsEditor.current?.serialize({
keepDeletedWidgets: true,
}),
},
);
return {
...this.itemEditor.current?.serialize({
keepDeletedWidgets: true,
}) ?? {},
hints: this.hintsEditor.current?.serialize({
keepDeletedWidgets: true,
},
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants